Skip to content

fix: Restore tar archive write performance regressed by padding trim#1719

Open
HofmeisterAn wants to merge 3 commits into
developfrom
bugfix/trim-tar-padding-keep-block-factor
Open

fix: Restore tar archive write performance regressed by padding trim#1719
HofmeisterAn wants to merge 3 commits into
developfrom
bugfix/trim-tar-padding-keep-block-factor

Conversation

@HofmeisterAn

@HofmeisterAn HofmeisterAn commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

The block factor of 1 introduced to fix the Podman EPIPE race (podman-container-tools/buildah#6678) was inadvertently applied to the Dockerfile build context path (DockerfileArchive), which writes to a FileStream on disk. At a block factor of 1, SharpZipLib flushes on every 512-byte block, significantly degrading image build performance.

/cc @Rob-Hague @artiomchi

Why is it important?

-

Related issues

Summary by CodeRabbit

  • Bug Fixes

    • Improved tar archive creation to reduce intermittent failures when generating or uploading tar-based build contexts in container environments.
  • Documentation

    • Expanded guidance on tar block factor usage, including why a value of 1 is recommended to avoid padding-related issues that can surface as broken-pipe errors, and cautioned against using the setting for Dockerfile build context due to performance impact.

@HofmeisterAn HofmeisterAn added the bug Something isn't working label Jun 29, 2026
@netlify

netlify Bot commented Jun 29, 2026

Copy link
Copy Markdown

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 66d0f6e
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-dotnet/deploys/6a4292719e47df00087f8eaf
😎 Deploy Preview https://deploy-preview-1719--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 87da3752-2472-4fd6-a574-004719fb4972

📥 Commits

Reviewing files that changed from the base of the PR and between fc26203 and 66d0f6e.

📒 Files selected for processing (1)
  • src/Testcontainers/TarArchiveDefaults.cs
✅ Files skipped from review due to trivial changes (1)
  • src/Testcontainers/TarArchiveDefaults.cs

Walkthrough

Removes the explicit TarArchiveDefaults.TarBlockFactor argument from DockerfileArchive.Tar, and expands the TarBlockFactor comment in TarArchiveDefaults to describe the Podman EPIPE race and its build-context warning.

Changes

TarBlockFactor cleanup and documentation

Layer / File(s) Summary
TarBlockFactor comment expansion and constructor fix
src/Testcontainers/TarArchiveDefaults.cs, src/Testcontainers/Images/DockerfileArchive.cs
TarArchiveDefaults.TarBlockFactor gains a longer Podman/archive race explanation and build-context warning, and DockerfileArchive.Tar stops passing TarBlockFactor explicitly to TarOutputStream.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

Poem

🐇 The tar stream hops along so light,
One block factor less, the path feels right.
A longer note for Podman’s race,
And archive winds now keep their pace.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: restoring tar archive write performance after a padding-trim regression.
Description check ✅ Passed The description covers the change and related issue context, but the Why section is effectively empty and would benefit from a clearer rationale.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bugfix/trim-tar-padding-keep-block-factor

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/Testcontainers/TarArchiveDefaults.cs`:
- Line 7: The comment in TarArchiveDefaults should be corrected to reflect the
actual padding size produced by the default factor. Update the wording around
the trailing zeros calculation so it states that a factor of 20 corresponds to
20 × 512 bytes, which is about 10 KB, not ~8 KB, and keep the explanation
aligned with the TarArchiveDefaults constant or related padding logic.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 404da95a-de65-4e21-bb7a-e91a46fe6206

📥 Commits

Reviewing files that changed from the base of the PR and between 3f97885 and fc26203.

📒 Files selected for processing (2)
  • src/Testcontainers/Images/DockerfileArchive.cs
  • src/Testcontainers/TarArchiveDefaults.cs

Comment thread src/Testcontainers/TarArchiveDefaults.cs Outdated
Comment thread src/Testcontainers/TarArchiveDefaults.cs
@Rob-Hague

Copy link
Copy Markdown
Contributor

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants